Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite to allow easier error handeling #3

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

LessThanGreaterThan
Copy link

I have rewritten parts of client to remove the req dependency as it makes error handling rather untransparent (which is also mentioned in #1 ) and switched to the inbuild net/http client.

I've also added support for following apis:

creating index
deleting index
ingesting data

this is still a WIP, looking for feedback if this is something to consider, and if so, what parts still need polishing.

Copy link

@pims pims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: I'm not part of the Quickwit team, just an internet rando.

This is a great start @LessThanGreaterThan ! Left a few minor comments and will provide more feedback once I've been able to use this client for local prototyping.

return nil
}),
endpoint: endpoint,
client: &http.Client{},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is often desirable to make the underlying http client configurable. What do you think about exposing both the endpoint and the client, so that they can be set/overridden externally?

type QuickwitClient struct {
	Endpoint string
	Client   *http.Client
}

The current

func NewQuickwitClient(endpoint string) *QuickwitClient{
  …
}

could stay as-is for now until more options are needed.

}
return nil, &errmsg
}
return &SearchResponse{}, nil
Copy link

@pims pims Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems that the quickwit response is never returned.
A small change like the following should get us a functional client! (tests needed of course)

diff --git a/client.go b/client.go
index 28b02c8..8da87ec 100644
--- a/client.go
+++ b/client.go
@@ -142,6 +142,7 @@ func (c *QuickwitClient) Search(indexId string, searchRequest SearchRequest) (*S
                return nil, err
        }
        defer post.Body.Close()
+
        if post.StatusCode != http.StatusOK {
                msg, err := io.ReadAll(post.Body)
                if err != nil {
@@ -154,7 +155,9 @@ func (c *QuickwitClient) Search(indexId string, searchRequest SearchRequest) (*S
                }
                return nil, &errmsg
        }
-       return &SearchResponse{}, nil
+       var searchResponse SearchResponse
+       err = json.NewDecoder(post.Body).Decode(&searchResponse)
+       return &searchResponse, err
 }

There's probably a way to reduce some amount of duplication across all methods but that doesn't have to be done today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants